CMake update to find liblz4 using pkg-config instead of cmake. #46

mholtrop · 2024-02-06T18:33:15Z

This is a request from Chris Dilks to improve the way the liblz4 library is found by the CMake scheme.
There is also a minor update of the Readme for the RDataFrame component.

Add first a pkg-config search, then fallback.

c-dilks · 2024-02-08T18:13:40Z

cmake/LZ4Config.cmake

-FIND_PACKAGE_HANDLE_STANDARD_ARGS(
-        LZ4 DEFAULT_MSG
-        LZ4_LIBRARY LZ4_INCLUDE_DIR LZ4_GOOD_VERSION)
+set(ENV{PKG_CONFIG_PATH} "${PKG_CONFIG_PATH}:${CMAKE_INSTALL_PREFIX}/lib/pkgconfig")


Suggested change

set(ENV{PKG_CONFIG_PATH} "${PKG_CONFIG_PATH}:${CMAKE_INSTALL_PREFIX}/lib/pkgconfig")

Is this needed? I think CMake's PkgConfig module will search CMAKE_PREFIX_PATH's lib/pkgconfig subdirectories by default, so you just need to make sure LZ4's top-level installation prefix is in CMAKE_PREFIX_PATH (and if LZ4 is installed at the system level, then it's not necessary to add the system directories since they will already be searched).

Well, "needed" it is not. If lz4 is properly installed it all works. However if the user installs lz4 from the directory included with hipo, then this line avoids the confusion with cmake not finding that install unless you also set the CMAKE_PREFIX_PATH. I would think that "expected behavior" is that cmake just finds the package files, as it would if we used a cmake install instead of pkgconfig. It seems a simple fix to me, but if anyone objects I am fine without it.

Sounds good to me

c-dilks · 2024-02-08T18:21:48Z

CMakeLists.txt

 endif()

 add_compile_definitions(__LZ4__)
-include_directories(${LZ4_INCLUDE_DIRS})
+include_directories(PkgConfig::LZ4)


I'm not sure if this works, since include_directories documentation states directory parameters, not targets; however, the Linux CI tests pass, so maybe it's okay.

The macOS tests still fail though:

/Users/runner/work/hipo/hipo/hipo4/recordbuilder.cpp:11:10: fatal error: 'lz4.h' file not found #include <lz4.h> ^~~~~~~ 1 error generated.

I wonder if you'd be better off adding PkgConfig::LZ4 to each of the relevant target_include_directories calls.

Yes, that may work better.
I am testing this on an older Intel Mac right now. It seems that the "include_directory" was not even needed on my current system.

On the machine in question, I would like to see the output of the command: "pkg-config --debug --cflags liblz4", and then an "ls" of the directory the result points to.
On my Intel Mac, I was able to re-create the error seen here because the pkg-config had a file in the /usr/local/lib/pkgconfig directory that pointed to a non-existing directory for liblz4. In other words, liblz4 was moved in some update, but the pkgconfig was not updated accordingly.
This seems to be a weakness of pkgconfig, that the config files can get out of sync with the reality on the hard drive.

Sounds like its pkg-config file may be non-relocatable, which I think is common when the assumption is that installed files won't be moved.

You can add whatever commands you want to the CI config (it will run every time you push a new commit). Either add them just before the cmake call or put them in a separate step:

hipo/.github/workflows/ci.yml

Lines 29 to 31 in dfacdce

cmake -S . -B build -DCMAKE_INSTALL_PREFIX=install

cmake --build build -j2

cmake --install build

Maybe mark this PR as draft so @gavalian doesn't merge this prematurely.

You can see my (unsuccessful) attempts here: c-dilks#2

I was doing some sanity checks to see if anything is out of date on the runner, but all the versions seem consistent with the Linux runner.

well.. I managed to get past the missing header file issue, but now the linker is complaining that it can't find liblz4:

https://github.com/c-dilks/hipo/actions/runs/7846867991/job/21414552371?pr=2

I'll be back online later this afternoon, but you can diff my branch with yours to see what I was trying.

ARGH! Okay, spend way too much time digging into the obscurities of cmake pkg-config implementation and how it is supposed to work. I still do not understand it quite, and why some things work on Intel Mac but not on Arm Mac and vise versa. I updated the code and now it works on both. I hope the tests pass now.

…t systems. ugh.

c-dilks · 2024-02-09T17:22:39Z

Got it to work on my fork. Here's the patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2ce2c85..f6aaac0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -75,7 +75,7 @@ if(NOT LZ4_FOUND)
 endif()

 add_compile_definitions(__LZ4__)
-include_directories(PkgConfig::LZ4)
+include_directories(${LZ4_INCLUDE_DIRS})

 set(DATAFRAME_IN_MAIN TRUE)
 add_subdirectory(extensions/dataframes)
@@ -126,7 +126,7 @@ set_target_properties(hipo4_static PROPERTIES OUTPUT_NAME hipo4)   # So that the

 # Required on Unix OS family to be able to be linked into shared libraries.
 set_target_properties(hipo4_objs PROPERTIES POSITION_INDEPENDENT_CODE ON)
-add_dependencies(hipo4_objs LZ4)
+add_dependencies(hipo4_objs PkgConfig::LZ4)

 # Include the macros for creating export package.
 include(CMakePackageConfigHelpers)
@@ -141,8 +141,8 @@ target_include_directories(hipo4 PUBLIC
                            $<INSTALL_INTERFACE:include/hipo4>
                            )

-target_link_libraries(hipo4 PUBLIC  ${LZ4_LIBRARIES} )
-target_link_libraries(hipo4_static PUBLIC ${LZ4_LIBRARIES})
+target_link_libraries(hipo4 PUBLIC PkgConfig::LZ4)
+target_link_libraries(hipo4_static PUBLIC PkgConfig::LZ4)

 install(TARGETS hipo4
         EXPORT hipo4-export
diff --git a/cmake/LZ4Config.cmake b/cmake/LZ4Config.cmake
index 943bf59..ed710de 100644
--- a/cmake/LZ4Config.cmake
+++ b/cmake/LZ4Config.cmake
@@ -3,13 +3,14 @@
 #
 set(ENV{PKG_CONFIG_PATH} "${PKG_CONFIG_PATH}:${CMAKE_INSTALL_PREFIX}/lib/pkgconfig")
 find_package(PkgConfig REQUIRED)
-pkg_check_modules(LZ4 liblz4)
+pkg_check_modules(LZ4 IMPORTED_TARGET liblz4)

 if (NOT LZ4_FOUND)
     message(STATUS "No system version of LZ4 found.")
 else()
     add_library(LZ4 INTERFACE IMPORTED)
     set_target_properties(LZ4 PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${LZ4_INCLUDE_DIR}")
-    message(STATUS "Found LZ4: ${LZ4_LIBRARIES}")
+    message(STATUS "Found LZ4 libraries: ${LZ4_LIBRARIES}")
+    message(STATUS "Found LZ4 headers: ${LZ4_INCLUDE_DIRS}")
     mark_as_advanced(LZ4_INCLUDE_DIRS LZ4_INCLUDE_DIR LZ4_LIBRARIES LZ4_LIBRARY)
 endif (NOT LZ4_FOUND)

Use pkgconfig for lz4

c-dilks · 2024-02-09T20:44:53Z

Looks good to me! CI tests pass both here and in iguana. Thank you very much for making this work.

@gavalian I believe this is ready for your review and merging.

gavalian

Looks good

mholtrop and others added 5 commits January 22, 2024 20:54

Add first a pkg-config search, then fallback.

0a8793c

Updated way to find and process lz4, thanks to Chris Dilks

13dca79

Still need to include the lz4 dir.

f0d3287

Minor update correcting bank names.

ab1fd4f

Merge pull request #2 from mholtrop/use_pkgconfig_for_lz4

c183ecf

Add first a pkg-config search, then fallback.

c-dilks reviewed Feb 8, 2024

View reviewed changes

Try to fix the LZ4 stuff for different behaviors of cmake on differen…

98d07bd

…t systems. ugh.

mholtrop and others added 3 commits February 9, 2024 13:16

Testing on Intel Mac

d220e0a

Cleanup. Seems to work in Intel Mac

9f8f4f1

Merge pull request #5 from mholtrop/use_pkgconfig_for_lz4

f64b58d

Use pkgconfig for lz4

c-dilks approved these changes Feb 9, 2024

View reviewed changes

c-dilks mentioned this pull request Feb 9, 2024

build: static link hipo4 JeffersonLab/iguana#74

Merged

gavalian approved these changes Feb 9, 2024

View reviewed changes

gavalian merged commit 87c3337 into gavalian:master Feb 9, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake update to find liblz4 using pkg-config instead of cmake. #46

CMake update to find liblz4 using pkg-config instead of cmake. #46

mholtrop commented Feb 6, 2024

c-dilks Feb 8, 2024

mholtrop Feb 9, 2024

c-dilks Feb 9, 2024

c-dilks Feb 8, 2024 •

edited

Loading

mholtrop Feb 9, 2024

mholtrop Feb 9, 2024

c-dilks Feb 9, 2024

c-dilks Feb 9, 2024

c-dilks Feb 9, 2024

mholtrop Feb 9, 2024

c-dilks commented Feb 9, 2024

c-dilks commented Feb 9, 2024

gavalian left a comment

	cmake -S . -B build -DCMAKE_INSTALL_PREFIX=install
	cmake --build build -j2
	cmake --install build

CMake update to find liblz4 using pkg-config instead of cmake. #46

CMake update to find liblz4 using pkg-config instead of cmake. #46

Conversation

mholtrop commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-dilks Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-dilks commented Feb 9, 2024

c-dilks commented Feb 9, 2024

gavalian left a comment

Choose a reason for hiding this comment

c-dilks Feb 8, 2024 •

edited

Loading